fix: require Fix entity for FIXED status on gated opport… - #3256
Open
anshulk-public wants to merge 3 commits into
Open
fix: require Fix entity for FIXED status on gated opport…#3256anshulk-public wants to merge 3 commits into
anshulk-public wants to merge 3 commits into
Conversation
…unity types Customers could PATCH a suggestion's status directly to FIXED via the public API with no corresponding Fix entity ever created, leaving orphaned FIXED suggestions with no audit trail of what fixed them. - Add SUGGESTION_TYPES_REQUIRING_FIX_ENTITY blocklist covering opportunity types where a Fix entity is expected before a suggestion can be FIXED. - patchSuggestion and patchSuggestionsStatus now reject (400) a direct transition to FIXED for these types, pointing callers at the fixes endpoint. - POST .../opportunities/:opportunityId/fixes (createFixes) and PATCH .../opportunities/:opportunityId/status (patchFixesStatus) now accept an optional suggestionsTargetStatus field: once a fix is successfully created/updated, its linked suggestions are atomically transitioned to that status, so a suggestion is never marked FIXED without a persisted Fix. - Add an ownership check on suggestionIds in createFixes (previously only patchFix validated this), and validate before creating the fix so an invalid suggestion ID fails fast without leaving an orphaned FixEntity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tus is set A Copilot review on the companion frontend PR flagged that the atomic suggestionsTargetStatus request updates a suggestion server-side but the frontend had no way to reflect that locally without a separate refetch, since createFixes/patchFixesStatus responses only ever returned the fix, never the suggestions it just transitioned. - createFixes and #patchFixStatus now capture bulkUpdateStatus's return value (previously discarded) and include it as an optional `suggestions` field on the response entry, serialized via SuggestionDto.toJSON. - The field is present only when suggestionsTargetStatus was provided and suggestions were actually transitioned as a result — callers that didn't mutate suggestion status get no suggestions field, and no extra DB read (getSuggestionsByFixEntityId) happens for callers that don't use the flag. - Updated FixOperationSuccess OpenAPI schema (shared by both endpoints' responses) to document the new optional field. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
This PR will trigger a patch release when merged. |
anshulk-public
temporarily deployed
to
dev-branches
September 10, 2026 09:17 — with
GitHub Actions
Inactive
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
anshulk-public
temporarily deployed
to
dev-branches
September 10, 2026 09:27 — with
GitHub Actions
Inactive
anshulk-public
requested review from
MysticatBot
and removed request for
MysticatBot
September 10, 2026 09:28
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Engineers are able to manually change a Suggestion's status to FIXED via the public
API without ever creating a Fix entity for it.
patchSuggestion(single) andpatchSuggestionsStatus(bulk) acceptstatus: FIXEDdirectly, with no check that acorresponding Fix exists. The only guards in place today — a generic status-transition
table (default
warn, notenforce) and an admin gate scoped toREJECTED— don'tknow or care whether a Fix backs the transition. Nowhere in this repo's
suggestion-status code paths is
Fix.createeven called; fix creation and suggestionstatus updates are entirely disjoint operations today.
We want to block this — but only for the opportunity types where a Fix entity is
actually expected to exist before a suggestion can be FIXED. Not every opportunity
type has a code path that produces a Fix (e.g. purely metric-derived resolutions), so
a blanket block would be wrong; it needs to be scoped to the types where "FIXED
without a Fix" is actually a data-integrity violation.
Jira: https://jira.corp.adobe.com/browse/SITES-50648
Constraint: fix this at the API layer, not the data-access layer
The obvious place to enforce "FIXED requires a Fix" as a hard invariant is the
data-access layer (
@adobe/spacecat-shared-data-access, inSuggestion.setStatus)— that's the one chokepoint every writer passes through. But that same data-access
layer is also used directly by the autofix worker (and potentially other future
consumers), not just this API. Changing the invariant there means changing behavior
for every consumer at once, in a separate package with its own release cycle, and
would need its own design pass (does the autofix worker's own internal flows always
satisfy the invariant already? does every opportunity type actually need it, or does
the enforcement need an escape hatch?).
We're keeping this change scoped to this repo's API layer only — the two public
PATCH endpoints — rather than the shared data-access layer. That means:
before marking FIXED, some do it in the reverse order with no atomicity) are a
separate, out-of-scope problem for a different repo.
every possible writer of
Suggestion.status.Solution
1. Block direct client transitions to FIXED, scoped by opportunity type.
A new constant,
SUGGESTION_TYPES_REQUIRING_FIX_ENTITY(
src/utils/suggestion-fix-required-types.js), lists exactly the opportunity typeswhere a Fix is expected before FIXED.
patchSuggestionandpatchSuggestionsStatuseach check: if the target status is FIXED and the suggestion's opportunity type is in
this list, reject with 400 and point the caller at the fixes endpoint instead. The
check is duplicated in both handlers (they're independent routes, neither delegates
to the other) — following the same pattern already used for the existing
REJECTED-transition check in both.
generic-opportunityis excluded from the listsince it's a shared fallback type used by several unrelated flows, not one semantic
type — blocking it would over-restrict suggestions that happen to share that
fallback bucket for unrelated reasons.
2. Give callers a real way to satisfy the requirement: create/update the Fix and
transition the suggestion atomically, in one request.
POST .../opportunities/:opportunityId/fixes(createFixes) andPATCH .../opportunities/:opportunityId/status(patchFixesStatus) now accept anoptional
suggestionsTargetStatusfield. When present, once the fix write succeeds,its linked suggestions are transitioned to that status in the same request — so a
suggestion is never transitioned without a fix already persisted behind it. Extending
these existing endpoints (rather than adding a new route) was the natural fit:
createFixesalready acceptedsuggestionIdsand already linked them; cascading astatus change on success is additive to what it already does, and
rollbackFailedFixin this same file already sets a precedent for this file touching suggestion status
as a side effect of a fix operation (it sets suggestions to SKIPPED on rollback). A
new route would just duplicate the existing access control, ownership checks, and
dedup logic for no semantic gain.
suggestionsTargetStatustakes an actual status value rather than a boolean flag(e.g.
markSuggestionsFixed: true), so the contract isn't hardcoded to FIXED anddoesn't need a second flag if some other status ever needs the same atomic guarantee.
Validation of the value itself isn't duplicated in this repo —
bulkUpdateStatusinthe shared data-access layer already throws on an invalid status, and the existing
error-mapping in both endpoints handles that.
3. This API is also what Success Studio UI calls for "mark as deployed" — so the
frontend flow needs to change too.
The UI's
createOpportunityFixes,setFixToDeployed, andsetStatusToDeployedScopedflows previously called this same fixes endpoint, then made a second, separate
patchSuggestionStatusPATCH call to flip the suggestion to FIXED — non-atomic, so afailure or race between the two calls could leave a suggestion FIXED with no Fix (the
exact bug this PR closes on the API side) or a Fix with no updated suggestion. That
frontend flow is updated in a companion PR (OneAdobe/experience-success-studio-ui#2277)
to pass
suggestionsTargetStatuson the same request instead of making a second call— since a customer-facing UI action was itself part of the original problem, it has to
move in lockstep with the API change, not be left calling the old two-step pattern
against a backend that will start rejecting the second step for gated types.
Test plan
npm test— full suite passingnpm run lint— cleannpm run docs:lint— valid, no new warnings(gated vs. non-gated type),
suggestionsTargetStatusbehavior on both fixendpoints (atomic success, absent-field no-op, ownership-check regression,
invalid-suggestion-ID fast-fail without orphaning a Fix)
🤖 Generated with Claude Code
Please ensure your pull request adheres to the following guidelines:
describe here the problem you're solving.
If the PR is changing the API specification:
yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
If the PR is changing the API implementation or an entity exposed through the API:
If the PR is introducing a new audit type:
Related Issues
Thanks for contributing!